feat: Create snapshots when running notebooks #256
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a snapshot feature for Deepnote notebooks. Users can now enable/disable snapshot mode via commands, which persists cell outputs to separate timestamped files in a snapshots folder. The main notebook file remains clean. The old SnapshotMetadataService was replaced with a new SnapshotService that handles both metadata tracking and file I/O. Serialization/deserialization logic was updated to read from and write to snapshot files when enabled. Cell execution queue now notifies on completion to trigger snapshot operations. The tree provider filters snapshot files from discovery. Sequence Diagram(s)sequenceDiagram
actor User
participant CmdListener as DeepnoteNotebookCommandListener
participant Config as IConfigurationService
participant Serializer as DeepnoteNotebookSerializer
participant SnapshotSvc as SnapshotService
participant FileSystem as FileSystem
User->>CmdListener: Enable Snapshots
CmdListener->>Config: updateSetting('deepnote.snapshots.enabled', true)
Config->>Config: persist configuration
Note over User,FileSystem: During Notebook Execution
User->>Serializer: saveNotebook()
Serializer->>Config: isSnapshotsEnabled()?
Config-->>Serializer: true
Serializer->>SnapshotSvc: createSnapshot(projectUri, projectId, projectName, projectData)
SnapshotSvc->>FileSystem: write snapshot file (timestamped)
FileSystem-->>SnapshotSvc: success
SnapshotSvc->>FileSystem: update latest snapshot pointer
FileSystem-->>SnapshotSvc: success
SnapshotSvc-->>Serializer: snapshotUri
Serializer->>Serializer: stripOutputsFromBlocks()
Serializer->>FileSystem: write main notebook (clean, no outputs)
FileSystem-->>Serializer: success
Note over User,FileSystem: During Notebook Open
User->>Serializer: loadNotebook()
Serializer->>Config: isSnapshotsEnabled()?
Config-->>Serializer: true
Serializer->>SnapshotSvc: readSnapshot(projectId)
SnapshotSvc->>FileSystem: read latest snapshot
FileSystem-->>SnapshotSvc: snapshot content
SnapshotSvc->>SnapshotSvc: parseSnapshotFile()
SnapshotSvc-->>Serializer: outputsMap
Serializer->>Serializer: mergeOutputsIntoBlocks(blocks, outputsMap)
Serializer-->>User: notebook with outputs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
package.jsonsrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/serviceRegistry.node.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/common/constants.ts
🧬 Code graph analysis (2)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
ISnapshotFileService(10-10)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (16)
src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
15-15: LGTM! Correct alphabetical ordering.Field reordering follows the coding guideline for private fields.
src/notebooks/serviceRegistry.node.ts (2)
92-92: LGTM.Import follows established pattern in this file.
250-250: LGTM.Correct DI registration for the snapshot file service.
src/platform/common/constants.ts (1)
227-228: LGTM.Command constants follow established patterns and naming conventions.
src/notebooks/deepnote/deepnoteActivationService.ts (1)
34-35: LGTM.Optional DI injection is correctly implemented, and serializer construction properly passes both snapshot services.
Also applies to: 45-49
src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-45: LGTM.Well-documented interface with clear method signatures. Follows established DI patterns with Symbol-based export.
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (1)
21-21: LGTM.Test scaffolding properly updated to accommodate IConfigurationService dependency. Mock implementation is appropriate and consistently applied.
Also applies to: 32-46, 91-91
package.json (1)
1648-1653: LGTM.Config property correctly defined with resource scope—appropriate for per-notebook snapshot settings.
src/notebooks/deepnote/snapshotFileService.unit.test.ts (3)
83-143: Good coverage of mergeOutputsIntoBlocks edge cases.Tests cover: ID matching, missing matches, empty maps, empty arrays.
145-226: Solid immutability testing.Line 194-208 correctly verifies original blocks aren't mutated—important for data integrity.
228-310: LGTM.Edge cases well-covered: missing IDs, missing outputs, complex output structures.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
143-146: LGTM.Constructor injection follows DI pattern correctly. Ordering alphabetical per guidelines.
src/notebooks/deepnote/deepnoteSerializer.ts (2)
60-62: LGTM.Optional injection with
@optional()decorator correctly used for gradual feature adoption.
394-405: LGTM.Helper correctly finds project URI and strips query params for clean path construction.
src/notebooks/deepnote/snapshotFileService.node.ts (2)
16-23: LGTM.Slugification handles edge cases: multiple spaces, leading/trailing hyphens.
175-178: May overwrite existing createdAt.Line 177 initializes metadata with new
createdAteven ifprojectData.metadataalready has one.-snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() }; +if (!snapshotData.metadata) { + snapshotData.metadata = { createdAt: new Date().toISOString() }; +}Likely an incorrect or invalid review comment.
| async createSnapshot( | ||
| projectUri: Uri, | ||
| projectId: string, | ||
| projectName: string, | ||
| projectData: DeepnoteFile | ||
| ): Promise<Uri | undefined> { | ||
| const latestPath = this.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
| const snapshotsDir = Uri.joinPath(latestPath, '..'); | ||
|
|
||
| // Ensure snapshots directory exists | ||
| try { | ||
| await workspace.fs.stat(snapshotsDir); | ||
| } catch { | ||
| logger.debug(`[SnapshotFileService] Creating snapshots directory: ${snapshotsDir.fsPath}`); | ||
| await workspace.fs.createDirectory(snapshotsDir); | ||
| } | ||
|
|
||
| // Check if there are changes compared to the existing latest snapshot | ||
| const hasChanges = await this.hasSnapshotChanges(latestPath, projectData); | ||
|
|
||
| if (!hasChanges) { | ||
| logger.debug(`[SnapshotFileService] No changes detected, skipping snapshot creation`); | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| const timestamp = generateTimestamp(); | ||
| const timestampedPath = this.buildSnapshotPath(projectUri, projectId, projectName, timestamp); | ||
|
|
||
| // Prepare snapshot data with timestamp | ||
| const snapshotData = structuredClone(projectData); | ||
|
|
||
| snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() }; | ||
| snapshotData.metadata.modifiedAt = new Date().toISOString(); | ||
|
|
||
| const yamlString = yaml.dump(snapshotData, { | ||
| indent: 2, | ||
| lineWidth: -1, | ||
| noRefs: true, | ||
| sortKeys: false | ||
| }); | ||
|
|
||
| const content = new TextEncoder().encode(yamlString); | ||
|
|
||
| // Write to timestamped file first (safe - doesn't touch existing files) | ||
| await workspace.fs.writeFile(timestampedPath, content); | ||
| logger.debug(`[SnapshotFileService] Wrote timestamped snapshot: ${timestampedPath.fsPath}`); | ||
|
|
||
| // Copy to latest (only after timestamped write succeeded) | ||
| await workspace.fs.writeFile(latestPath, content); | ||
| logger.debug(`[SnapshotFileService] Updated latest snapshot: ${latestPath.fsPath}`); | ||
|
|
||
| return timestampedPath; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Race condition between timestamped and latest writes.
If process crashes after line 190 but before line 194, latest snapshot becomes stale. Consider using a temp file and atomic rename for the latest file.
Artmann
changed the title
Artmann
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 217-257: The fallback branch when findProjectUriFromId(projectId)
returns falsy silently assigns notebook.blocks but lacks logging; add a debug
log via logger.debug inside the else that mentions projectId and that snapshot
URI was unavailable (e.g., "SerializeNotebook: projectUri not found for
projectId={projectId}, writing full outputs to main file"), so the fallback path
in the snapshot handling around projectUri,
snapshotFileService.isSnapshotsEnabled(), and notebook.blocks is observable for
troubleshooting.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 17-24: slugifyProjectName can return an empty string when the
input name contains only non-alphanumeric characters; update slugifyProjectName
to detect an empty result and return a sensible fallback (e.g., a default slug
like "untitled-project" or a derived fallback such as a truncated hash/UUID) so
callers never receive an empty slug. Locate the slugifyProjectName function and
after the existing chain of replacements, check if the resulting string is empty
and if so return the chosen fallback value; ensure the fallback is deterministic
or clearly documented depending on needs.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteSerializer.ts (3)
src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(77-77)ISnapshotMetadataServiceFull(79-111)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
error(30-37)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (18)
package.nls.json (1)
253-254: LGTM!New i18n entries follow existing conventions.
package.json (2)
102-111: LGTM!Commands registered correctly with proper i18n references.
1648-1653: LGTM!Configuration uses
"scope": "resource"enabling per-workspace control.src/notebooks/deepnote/deepnoteActivationService.ts (2)
34-35: LGTM!Optional injection allows graceful degradation when snapshot services unavailable.
45-49: LGTM!Serializer correctly receives all three dependencies.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
143-146: LGTM!Constructor injects required services correctly.
188-191: LGTM!Commands registered and wired to handlers.
380-408: Setting key prefix is correct.updateSetting('snapshots.enabled', ...)correctly relies onIConfigurationServiceprepending'deepnote.'prefix—the implementation explicitly callsupdateSectionSetting('deepnote', setting, ...), resulting in the correctdeepnote.snapshots.enabledconfiguration key.src/notebooks/deepnote/deepnoteSerializer.ts (3)
60-61: LGTM!Optional dependency follows existing pattern.
119-134: LGTM!Reads snapshot outputs and merges before cell conversion.
structuredClonepreserves original blocks.
395-406: LGTM!Strips query params to get clean project URI for snapshot paths.
src/notebooks/deepnote/snapshotFileService.node.ts (7)
39-47: LGTM!Reads configuration with safe default.
56-67: LGTM!Uses
Uri.joinPathper coding guidelines.
75-114: LGTM!Fallback from latest to timestamped is sound. Lexicographic sort works for ISO timestamps.
146-199: LGTM!Atomic write pattern: timestamped first, then latest. Directory creation handles missing folder.
261-268: LGTM!Returns new array; original untouched.
244-255: In-place mutation is already properly handled.The method does mutate blocks in place, and the doc comment documents this. The only production caller in
deepnoteSerializer.tsalready passes a cloned array viastructuredClone()before calling the method. No action required.
205-226: Key ordering is deterministic here—no risk of false positives.
getComparableProjectContent()explicitly constructs an object with fixed key order (version→project→environment→execution), and both objects are processed identically. Modern JavaScript preserves insertion order, soJSON.stringify()will produce consistent output. Also, the method doesn't includenotebooksas mentioned in the concern; keys come from the explicit object literal.Likely an incorrect or invalid review comment.
| // Handle snapshot file logic if enabled | ||
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | ||
| const projectUri = this.findProjectUriFromId(projectId); | ||
|
|
||
| if (projectUri) { | ||
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | ||
|
|
||
| // Create snapshot project with full outputs | ||
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | ||
| const snapshotNotebook = snapshotProject.project.notebooks.find( | ||
| (nb: { id: string }) => nb.id === notebookId | ||
| ); | ||
|
|
||
| if (snapshotNotebook) { | ||
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } | ||
|
|
||
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | ||
| const snapshotUri = await this.snapshotFileService.createSnapshot( | ||
| projectUri, | ||
| projectId, | ||
| originalProject.project.name, | ||
| snapshotProject | ||
| ); | ||
|
|
||
| // Strip outputs from main file blocks | ||
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | ||
|
|
||
| if (snapshotUri) { | ||
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | ||
| } else { | ||
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | ||
| } | ||
| } else { | ||
| // Fallback if we can't find the project URI | ||
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } | ||
| } else { | ||
| // Default behavior: outputs in main file | ||
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging the fallback path.
Line 252: Silent fallback when projectUri not found. Adding a debug log would aid troubleshooting.
Suggested change
} else {
// Fallback if we can't find the project URI
+ logger.debug('SerializeNotebook: Could not find project URI, skipping snapshot');
notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle snapshot file logic if enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const projectUri = this.findProjectUriFromId(projectId); | |
| if (projectUri) { | |
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | |
| // Create snapshot project with full outputs | |
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | |
| const snapshotNotebook = snapshotProject.project.notebooks.find( | |
| (nb: { id: string }) => nb.id === notebookId | |
| ); | |
| if (snapshotNotebook) { | |
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | |
| const snapshotUri = await this.snapshotFileService.createSnapshot( | |
| projectUri, | |
| projectId, | |
| originalProject.project.name, | |
| snapshotProject | |
| ); | |
| // Strip outputs from main file blocks | |
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | |
| if (snapshotUri) { | |
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | |
| } else { | |
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | |
| } | |
| } else { | |
| // Fallback if we can't find the project URI | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| } else { | |
| // Default behavior: outputs in main file | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Handle snapshot file logic if enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const projectUri = this.findProjectUriFromId(projectId); | |
| if (projectUri) { | |
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | |
| // Create snapshot project with full outputs | |
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | |
| const snapshotNotebook = snapshotProject.project.notebooks.find( | |
| (nb: { id: string }) => nb.id === notebookId | |
| ); | |
| if (snapshotNotebook) { | |
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | |
| const snapshotUri = await this.snapshotFileService.createSnapshot( | |
| projectUri, | |
| projectId, | |
| originalProject.project.name, | |
| snapshotProject | |
| ); | |
| // Strip outputs from main file blocks | |
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | |
| if (snapshotUri) { | |
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | |
| } else { | |
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | |
| } | |
| } else { | |
| // Fallback if we can't find the project URI | |
| logger.debug('SerializeNotebook: Could not find project URI, skipping snapshot'); | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| } else { | |
| // Default behavior: outputs in main file | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteSerializer.ts around lines 217 - 257, The
fallback branch when findProjectUriFromId(projectId) returns falsy silently
assigns notebook.blocks but lacks logging; add a debug log via logger.debug
inside the else that mentions projectId and that snapshot URI was unavailable
(e.g., "SerializeNotebook: projectUri not found for projectId={projectId},
writing full outputs to main file"), so the fallback path in the snapshot
handling around projectUri, snapshotFileService.isSnapshotsEnabled(), and
notebook.blocks is observable for troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 212-233: The JSON.stringify comparison in hasSnapshotChanges is
brittle due to property order; replace the string comparison with a proper deep
equality check by importing and using a stable deep-equal function (e.g.,
lodash.isEqual or fast-deep-equal) to compare the results of
getComparableProjectContent(existingData) and
getComparableProjectContent(projectData); keep the existing try/catch and
behavior (return true on read error) but change the return to use
deepEqual(existingProject, newProject) negated instead of comparing JSON
strings.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/snapshotFileService.unit.test.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteBlock(14-14)DeepnoteOutput(15-15)
src/notebooks/deepnote/snapshotFileService.node.ts (2)
src/notebooks/deepnote/deepnoteSerializer.ts (4)
injectable(54-451)DeepnoteOutput(15-15)DeepnoteFile(14-14)DeepnoteBlock(14-14)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (12)
src/notebooks/deepnote/snapshotFileService.unit.test.ts (1)
1-344: Comprehensive test coverage.The test suite thoroughly validates path building, output merging/stripping/extraction, edge cases, and error scenarios. Well-structured and follows coding guidelines.
src/notebooks/deepnote/snapshotFileService.node.ts (11)
18-31: LGTM.Slugification logic correctly handles spaces, special characters, and validates non-empty results.
37-39: LGTM.Timestamp generation is straightforward and produces filesystem-safe filenames.
50-54: LGTM.Configuration reading is straightforward.
63-74: LGTM.Path construction correctly uses
Uri.joinPath()per coding guidelines.
82-121: LGTM.Snapshot discovery logic correctly prioritizes latest snapshot, falls back to newest timestamped file, and handles sorting/filtering properly.
126-144: LGTM.Snapshot parsing safely uses optional chaining and validates block IDs and outputs.
153-206: LGTM.Atomic write pattern (timestamped first, then latest) ensures data integrity. Change detection prevents unnecessary writes.
238-245: LGTM.Correctly excludes timestamp metadata from comparison.
251-262: LGTM.In-place mutation correctly matches the interface contract and is well-tested.
268-275: LGTM.Immutable output stripping via destructuring is clean and correct.
281-291: LGTM.Extraction logic correctly validates block IDs and outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 77-122: The readSnapshot method lacks error handling around
parseSnapshotFile and searches all workspace roots with an expensive recursive
glob; wrap both calls to this.parseSnapshotFile(latestFiles[0]) and
this.parseSnapshotFile(newestFile) in try-catch blocks that log the error (using
logger.error) and return undefined (or a well-documented failure value) instead
of letting exceptions bubble; scope workspace.findFiles by passing a
RelativePattern (or a workspaceFolder argument) instead of a bare glob so
results are limited to the intended project workspace, and tighten the glob
(e.g., remove the leading **/ or target the snapshots/ directory directly) to
reduce cost—update references: readSnapshot, latestPattern, timestampedPattern,
workspace.findFiles, parseSnapshotFile, and sortedFiles accordingly.
- Around line 124-145: parseSnapshotFile currently assumes yaml.load succeeds
and snapshotData has the expected shape, which can crash on malformed YAML or
unexpected structure; wrap the file read + yaml.load in a try/catch, log the
parsing error via logger.error including the caught error, and either rethrow a
descriptive Error or return an empty outputsMap; after parsing, validate that
snapshotData is an object and snapshotData.project?.notebooks is an array before
iterating (skip or log and return empty map if not), and ensure block and
block.outputs are type-checked before calling outputsMap.set so malformed
structures don't throw.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (8)
src/notebooks/deepnote/snapshotFileService.node.ts (8)
14-32: LGTM - robust slugification.Handles edge cases and throws on empty results. Combined with projectId in filenames prevents collisions.
34-40: LGTM.
48-55: LGTM.
57-75: LGTM - follows Uri.joinPath guidelines.
209-231: LGTM - appropriate change detection.Correctly excludes metadata timestamps and treats read failures as changed state.
233-243: LGTM.
245-260: LGTM.
262-289: LGTM - clean utility methods.Both methods correctly handle their transformations without side effects.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
57-68: Add the Disable Snapshots command to the command palette table.The PR includes both
Deepnote: Enable SnapshotsandDeepnote: Disable Snapshotscommands, but the table documents only the enable command. Add a row for the disable command to maintain consistency with the feature's symmetric interface.📝 Proposed addition to the command table
| `Notebook: Change Cell Language` | Change the language of the cell currently in focus | +| `Deepnote: Disable Snapshots` | Disable snapshot mode for the current workspace | | `Deepnote: Enable Snapshots` | Enable snapshot mode for the current workspace |
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 119-134: Wrap the snapshot read/merge block in a try/catch so
failures in snapshotFileService.readSnapshot or
snapshotFileService.mergeOutputsIntoBlocks don't crash deserialization: catch
errors around the existing snapshot logic (the readSnapshot call and
mergeOutputsIntoBlocks invocation), log the exception via logger with context
(e.g., include projectId and that merging failed), and fall back to the original
cells computed from this.converter.convertBlocksToCells(selectedNotebook.blocks
?? []) so the function returns the baseline cells when snapshot handling fails.
In @src/notebooks/deepnote/deepnoteTreeDataProvider.ts:
- Line 206: Extract the repeated string '.snapshot.deepnote' into a single
constant (e.g. SNAPSHOT_FILE_SUFFIX) declared near the top of the file, then
replace all literal occurrences with that constant—specifically update the
projectFiles filter (projectFiles = files.filter(...)) and the three
file-watcher guard checks so they use SNAPSHOT_FILE_SUFFIX.endsWith/endsWith
comparisons instead of the repeated literal.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 318-380: updateLatestSnapshot duplicates directory creation,
change detection, metadata handling and YAML serialization found in
createSnapshot; extract the shared logic into a helper (e.g.
prepareSnapshotData) that takes (projectUri, projectId, projectName,
projectData) and returns { latestPath, content } or undefined when no changes,
move the directory creation into an ensureDirectoryExists helper used by both,
reuse hasSnapshotChanges and buildSnapshotPath inside the helper, and then have
updateLatestSnapshot call the helper and only perform the final
workspace.fs.writeFile(latestPath, content) and logging.
- Around line 19-32: Replace the generic Error thrown in slugifyProjectName with
a typed error from src/platform/errors: import the appropriate class (e.g.,
ValidationError or InvalidInputError) and throw that (e.g., throw new
ValidationError('Project name cannot be empty or contain only special
characters')) so callers can programmatically detect this validation failure;
update the import list at the top of the file and keep the message intact.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
README.mdsrc/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotMetadataService.ts
src/kernels/{kernel.ts,kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use event-driven updates via
EventEmitterfor kernel state changes and lifecycle events (willRestart, willInterrupt, onDidChangeKernels, etc.)
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/{kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use weak references or proper cleanup patterns to avoid memory leaks with notebook references in kernel tracking and lifecycle management
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/execution/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Coordinate notebook-level execution through
NotebookKernelExecutionwhich managesCellExecutionQueuefor sequential cell execution and usesCellExecutionMessageHandlerto process kernel messages
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/{kernel.ts,kernelFinder.ts,execution/*.ts,**/launcher*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Add telemetry tracking for all kernel operations (discovery, startup, execution, errors) to enable usage analytics
Files:
src/kernels/execution/cellExecutionQueue.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotMetadataService.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotMetadataService.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotMetadataService.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/notebooks/deepnote/types.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (5)
src/kernels/execution/cellExecutionQueue.ts (2)
src/platform/logging/outputChannelLogger.ts (1)
error(33-35)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/notebookCommandListener.ts (2)
src/kernels/kernelStatusProvider.ts (2)
IKernelStatusProvider(14-14)IKernelStatusProvider(15-17)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-157)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotMetadataService.ts (3)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
DeepnoteDataConverter(37-560)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteFile(14-14)DeepnoteBlock(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (19)
README.md (1)
92-108: Snapshot mode documentation is clear and well-structured.The explanation of how snapshots work, the benefits for Git workflows, and the enablement steps are clear and align with the feature's design.
src/platform/notebooks/deepnote/types.ts (1)
139-156: LGTM: Clear interface additions for Run All snapshot workflow.The three new methods properly extend the service interface with clear documentation. Using string-based URIs maintains consistency with the existing
captureEnvironmentBeforeExecutionmethod.src/kernels/execution/cellExecutionQueue.ts (1)
328-335: LGTM: Post-execution hook properly integrated.The hook correctly notifies the snapshot service after cell execution completes. Error handling with
logger.warnis appropriate, and the null checks prevent issues when snapshot service isn't available.src/notebooks/notebookCommandListener.ts (2)
58-59: Verify: Required injection may cause issues if service unavailable.The
ISnapshotMetadataServiceis injected as a required dependency (no@optional()decorator). This means the service must always be registered, or DI will fail. Consider making it optional if snapshots are an optional feature.Based on coding guidelines, use
@optional()for optional platform features:@inject(ISnapshotMetadataService) @optional() private snapshotMetadataService?: ISnapshotMetadataServiceThen add a null check before usage at Line 121.
119-121: LGTM: Run All mode set before execution.The snapshot service is correctly notified before executing the notebook. The synchronous call is appropriate since
setRunAllModeis a void method per the interface.src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
206-208: LGTM: Snapshot files filtered from tree view.The filtering correctly excludes snapshot files from the project tree, and Line 208 properly iterates over the filtered set rather than all files.
src/notebooks/deepnote/deepnoteSerializer.ts (2)
218-225: LGTM: Conditional output handling for snapshot mode.The serializer correctly strips outputs when snapshots are enabled, preserving the original behavior otherwise. The comment clarifies that snapshots are created during execution, not on save.
60-61: LGTM: Optional dependency injection pattern.Using
@optional()forISnapshotFileServicecorrectly handles the case where snapshots may not be available. This follows the established pattern in Line 60 forsnapshotService.src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-57: Well-designed interface.Clean separation of concerns. Method signatures correctly distinguish between full snapshots (timestamped + latest) and partial updates (latest only). JSDoc comments are helpful.
src/notebooks/deepnote/snapshotMetadataService.ts (5)
126-128: LGTM.Direct instantiation of
DeepnoteDataConverteris fine for a stateless utility class.
130-135: Optional injection pattern looks correct.Graceful degradation when snapshot service is unavailable.
547-553: Works only for open notebooks.This searches
workspace.notebookDocuments, so it only finds URIs for currently open notebooks. Fine here since it's called during active execution.
276-310: Snapshot branching logic is sound.Run All → full snapshot with history. Partial run → latest only. Silent failures with logging are acceptable for this non-critical feature.
259-266: No issues found. TheconvertCellsToBlocksmethod exists onDeepnoteDataConverterand is correctly used at line 266.src/notebooks/deepnote/snapshotFileService.node.ts (5)
83-153: Snapshot discovery logic is correct.Latest first, fallback to newest timestamped. The 100-file limit is reasonable.
386-416: Change detection is correct.Using
fast-deep-equalwith metadata exclusion ensures timestamps don't trigger false positives.
422-433: LGTM.In-place mutation is documented. Correct merge logic.
439-446: Clean output stripping.Immutable operation via rest spread. Type assertion is necessary here.
452-462: LGTM.Correctly filters blocks without id or outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteTreeDataProvider.ts:
- Around line 323-325: Duplicate snapshot-file guard logic across the file
watcher handlers (onDidChange, onDidCreate, onDidDelete) should be extracted
into a private helper to reduce repetition: add a method (e.g., private
isSnapshotFile(uri: Uri): boolean) that checks
uri.path.endsWith(SNAPSHOT_FILE_SUFFIX) and replace the inline checks in the
three handlers with calls to this helper, returning early when true; update
references in deepnoteTreeDataProvider.ts to use isSnapshotFile instead of
repeating the endsWith check.
In @src/notebooks/deepnote/errors.ts:
- Around line 6-11: The InvalidProjectNameError class is defined as a plain
Error with a hardcoded user-facing message but is used across modules; move the
InvalidProjectNameError definition into the shared platform errors module where
other BaseError subclasses live, change it to extend BaseError, wrap the default
message with l10n.t('Project name cannot be empty or contain only special
characters'), and call super({ message }) in the constructor while retaining
this.name = 'InvalidProjectNameError' so existing imports (e.g. usages in
snapshotFileService.node.ts) keep working.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 20-33: slugifyProjectName currently assumes name is a non-empty
string and will throw or behave unpredictably for null/undefined or non-string
inputs; add an input guard at the start of slugifyProjectName that checks typeof
name === 'string' and that name.trim() is non-empty, and if not, throw
InvalidProjectNameError (or coerce to string only after validation); then
continue using the trimmed value for the subsequent transformations to avoid
runtime errors on invalid inputs.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cspell.jsonsrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/errors.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/notebooks/deepnote/errors.ts (1)
InvalidProjectNameError(6-11)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
🔇 Additional comments (20)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
21-22: LGTM: Clear constant definition.Constant naming and documentation are appropriate for filtering snapshot files from the explorer view.
209-211: LGTM: Filtering logic is correct.Snapshot files are properly excluded from project file discovery, and the loop correctly iterates over the filtered list.
cspell.json (1)
81-84: LGTM!Spell-check vocabulary expansion aligns with new slugification utilities.
src/notebooks/deepnote/deepnoteSerializer.ts (4)
11-11: LGTM!Import follows established patterns for local file imports.
60-61: LGTM!Optional injection enables graceful degradation when snapshot feature is disabled.
224-233: LGTM!Snapshot mode correctly strips outputs from main file. Note: different cloning strategies (structuredClone vs. cloneWithoutCircularRefs) are appropriate for their respective contexts.
119-142: Use availability check forstructuredClonelike deepnoteExplorerView.ts does.Line 130 uses
structuredClonewithout verifying availability. The same module'sdeepnoteExplorerView.tsalready implements a safe pattern—checktypeof structuredClone !== 'undefined'and fall back toJSON.parse(JSON.stringify())for broader Node.js compatibility.src/notebooks/deepnote/snapshotFileService.node.ts (13)
52-56: LGTM!Configuration check is straightforward and correct.
65-76: LGTM!Path construction follows coding guidelines using
Uri.joinPath().
84-154: LGTM!Fallback logic (latest → newest timestamped) is well-structured with appropriate error handling. Note: 100-file limit at line 121 is reasonable but could be reached in long-lived projects.
221-242: LGTM!Directory creation with proper error handling and boolean success indicator.
249-288: LGTM!Snapshot preparation logic is sound. Note:
structuredCloneat line 273 requires Node 17+ (same consideration as deepnoteSerializer.ts).
302-348: LGTM!Two-write strategy (timestamped first, then latest) provides good safety guarantees. Fallback logic in
readSnapshotcorrectly handles stale 'latest' pointers.
356-383: LGTM!Latest-only update for partial execution is correctly implemented.
389-407: LGTM!Change detection using
fastDeepEqualwith timestamp-stripped comparison is appropriate.
412-419: LGTM!Strips metadata to prevent timestamp-driven false positives.
425-436: LGTM!In-place output merging is correctly implemented with debug logging.
442-449: LGTM!Output stripping via destructuring is clean and non-mutating.
455-465: LGTM!Output extraction into Map is straightforward and correct.
160-215: This recommendation is based on outdated js-yaml patterns. In v4 (which the project uses),yaml.load()is safe by default—safeLoad()was removed. The current code already follows the correct approach.Likely an incorrect or invalid review comment.
src/notebooks/deepnote/errors.ts
Outdated
| export class InvalidProjectNameError extends Error { | ||
| constructor(message: string = 'Project name cannot be empty or contain only special characters') { | ||
| super(message); | ||
| this.name = 'InvalidProjectNameError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other custom error classes to understand the pattern
echo "=== Custom error classes in codebase ==="
rg -n "class \w+Error extends Error" --type ts -C 2 | head -100
echo -e "\n=== Errors using l10n.t() ==="
rg -n "l10n\.t\(" --type ts -g "*error*.ts" -C 2
echo -e "\n=== Check src/platform/errors/ structure ==="
find src/platform/errors -name "*.ts" 2>/dev/null | head -20
echo -e "\n=== Check for InvalidProjectNameError references ==="
rg -n "InvalidProjectNameError" --type tsRepository: deepnote/vscode-deepnote
Length of output: 6968
Move error to src/platform/errors/ and localize the message.
This exported error is used across modules (src/notebooks/deepnote/snapshotFileService.node.ts) so it belongs in src/platform/errors/ per coding guidelines. The default message is user-facing and should be wrapped in l10n.t():
export class InvalidProjectNameError extends BaseError {
constructor(message: string = l10n.t('Project name cannot be empty or contain only special characters')) {
super({ message });
this.name = 'InvalidProjectNameError';
}
}🤖 Prompt for AI Agents
In @src/notebooks/deepnote/errors.ts around lines 6 - 11, The
InvalidProjectNameError class is defined as a plain Error with a hardcoded
user-facing message but is used across modules; move the InvalidProjectNameError
definition into the shared platform errors module where other BaseError
subclasses live, change it to extend BaseError, wrap the default message with
l10n.t('Project name cannot be empty or contain only special characters'), and
call super({ message }) in the constructor while retaining this.name =
'InvalidProjectNameError' so existing imports (e.g. usages in
snapshotFileService.node.ts) keep working.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
210-259: Excludesnapshots/infindFiles(don’t filter after the fact).This avoids scanning snapshot files at all and keeps the intent centralized.
Proposed diff
for (const workspaceFolder of workspace.workspaceFolders || []) { const pattern = new RelativePattern(workspaceFolder, '**/*.deepnote'); - const files = await workspace.findFiles(pattern); - const projectFiles = files.filter((file) => !file.path.endsWith(SNAPSHOT_FILE_SUFFIX)); + const files = await workspace.findFiles(pattern, '**/snapshots/**'); + const projectFiles = files.filter((file) => !isSnapshotFile(file)); for (const file of projectFiles) { try {
329-353: Watcher no-op for snapshot files is the right behavior.Optional: if VS Code glob negation is reliable in this extension’s supported versions, consider excluding
**/snapshots/**at the watcher level too (less event churn).
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 306-352: In createSnapshot, instead of writing directly to
latestPath (which risks partial-write), write the snapshot content to a
temporary file in the same directory (e.g., latestPath + '.tmp' or include a
unique suffix using timestamp) using workspace.fs.writeFile, then atomically
rename/move that temp file to latestPath using workspace.fs.rename; on rename
failure log a warning (including error and timestampedPath) and ensure any
leftover temp file is cleaned up if possible; keep returning timestampedPath
even if latest update fails and preserve the existing timestamped write/err
handling around the timestampedPath and latestPath operations.
- Around line 15-37: slugifyProjectName currently throws InvalidProjectNameError
which can bubble up and abort snapshot/save flows; change slugifyProjectName to
NOT throw: validate input and on failure log a warning and return null (or
undefined) instead of throwing, and then update all callers that assume a string
(the other slugify usages in this file) to check the return value and skip/log
and continue the snapshot/save flow when slug is null; leave the
InvalidProjectNameError class for other contexts but stop using it here so
failures are handled locally.
- Around line 225-246: The current ensureSnapshotsDirectory uses
workspace.fs.stat(snapshotsDir) and treats any success as the directory
existing, but if stat points to a non-directory file later writes will fail;
change ensureSnapshotsDirectory to call workspace.fs.stat(snapshotsDir), inspect
the returned FileStat.type (using the vscode FileType enum) and only return true
when the type includes FileType.Directory; if the path exists but is not a
directory, log a clear error and either remove/replace it with a directory (or
return false) before attempting workspace.fs.createDirectory(snapshotsDir); keep
existing error logging paths (logger.debug/logger.error) and references to
snapshotsDir and ensureSnapshotsDirectory.
- Around line 429-440: The assignment block.outputs = outputs.get(block.id) can
break strict TypeScript because Map.get returns T | undefined even after
outputs.has(block.id) — change the code in mergeOutputsIntoBlocks to first fetch
into a local const (e.g., const outs = outputs.get(block.id)) and then assign
only after a runtime check (if (outs) block.outputs = outs), or use a non-null
assertion (outputs.get(block.id)!) if you prefer; reference the
mergeOutputsIntoBlocks function, the outputs Map, and the block.outputs property
when making the change.
- Around line 88-158: The readSnapshot method logs absolute paths (fsPath) which
can contain PII; update every logger.debug and logger.error in readSnapshot to
avoid printing uri.fsPath directly and instead log a non-sensitive identifier
such as Utils.basename(uri) or a redacted form (e.g.,
`<redacted>/${Utils.basename(uri)}`) for latestFiles[0], newestFile and any uri
used in messages, and include the error object (when present) without exposing
the full path; adjust messages around the latest snapshot detection, timestamped
snapshot selection, and parse failures (references: readSnapshot, latestFiles,
latestPattern, newestFile, allTimestampedFiles, parseSnapshotFile).
- Around line 164-219: The YAML parsing in parseSnapshotFile is using yaml.load
without a safe schema or size checks; update parseSnapshotFile to first enforce
a maximum content size (e.g., reject/return if contentString.length or the raw
byte length exceeds a reasonable limit such as 1MB) and then call yaml.load with
an explicit safe schema option (e.g., { schema: yaml.SAFE_SCHEMA }) to prevent
arbitrary code execution; apply the same changes to the hasSnapshotChanges
parsing call around line 397 and any other YAML.parse/load usages to standardize
safe parsing and consistent size limits, and log a clear error when the document
is too large or fails safe parsing.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/platform/errors/invalidProjectNameError.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/platform/errors/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/errors/invalidProjectNameError.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/platform/errors/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/platform/errors/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (3)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
21-29: Snapshot detection helper looks fine.src/notebooks/deepnote/snapshotFileService.node.ts (1)
253-293:structuredClone()runtime support needs confirming.src/platform/errors/invalidProjectNameError.ts (1)
17-21: Use a specific error category instead of'unknown', but the proposed change is incomplete.The first parameter to
BaseErroriscategory(notcode), and it must be one of theErrorCategoryunion values defined intypes.ts. The suggested value'invalidProjectName'is not in that union. To implement this fix, add'invalidProjectName'to theErrorCategorytype insrc/platform/errors/types.ts, then update the error class accordingly.
Resolve conflict in deepnoteNotebookCommandListener.ts: - Keep text block commands from main - Keep snapshot enable/disable commands from branch
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 494-504: In extractOutputsFromBlocks, don't blindly cast
block.outputs to DeepnoteOutput[]; instead validate that block.outputs is an
array (Array.isArray) and that each element has the expected shape (e.g., is an
object and contains the required output fields used elsewhere), filter/transform
only the valid entries into a DeepnoteOutput[] before calling
outputsMap.set(block.id, ...), and skip or log blocks with malformed outputs so
you never insert invalid types into the map (refer to extractOutputsFromBlocks
and block.outputs when implementing the checks).
- Around line 207-216: The loop that sets outputsMap currently checks
Array.isArray(block.outputs) then casts to DeepnoteOutput[] without validating
items; add a runtime type guard (e.g., an isDeepnoteOutput(item): boolean) and
only include items that pass the guard (or skip the whole block if the outputs
array contains invalid entries), then set outputsMap.set(block.id,
validatedOutputs) instead of blind casting; alternatively, if you intentionally
trust snapshots, add an inline comment documenting that assumption and add a
defensive log/error when malformed outputs are encountered so corruption is
detectable.
- Around line 128-142: The code currently concatenates all results from
workspace.findFiles into allTimestampedFiles and then sorts them, which can be
slow and memory-heavy for many snapshots; instead introduce a configurable
maxSnapshots (or maxSnapshotsPerFolder) and when iterating results from
workspace.findFiles(timestampedPattern) maintain a fixed-size min-heap (priority
queue) keyed by Utils.basename(filename) to keep only the top N most-recent
timestamped files while filtering out URIs with '_latest.'; after processing all
folders extract and reverse the heap to produce sortedFiles; alternatively
expose the limit via config and document snapshot retention policy if you prefer
increasing the limit rather than streaming with a heap.
- Around line 304-305: Initialize createdAt when missing instead of only when
metadata is falsy: ensure snapshotData.metadata exists (leave existing object
intact), then if snapshotData.metadata.createdAt is undefined set it to new
Date().toISOString(), and always set snapshotData.metadata.modifiedAt to new
Date().toISOString(); update the code around snapshotData.metadata
initialization in the snapshot handling logic so you don't overwrite an existing
metadata object but still populate a missing createdAt and update modifiedAt.
- Around line 134-142: The filter on allTimestampedFiles is too broad because it
checks uri.fsPath.includes('_latest.') and will exclude files when the project
slug contains "_latest"; instead inspect the filename/variant portion only: use
Utils.basename(uri) (as already used later) to extract the filename and test
whether the filename's variant segment equals or contains "_latest" (e.g., parse
the basename into name/variant/timestamp parts) and filter out only when that
variant indicates the latest snapshot; update the filter in the sortedFiles
creation to operate on Utils.basename(uri) rather than uri.fsPath so only true
"_latest" snapshot variants are excluded.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🔇 Additional comments (8)
src/notebooks/deepnote/snapshotFileService.node.ts (8)
15-45: LGTM!Utility functions are well-implemented with proper validation and error handling.
53-60: LGTM!Configuration reading is straightforward.
69-80: LGTM!Path construction follows guidelines with proper use of
Uri.joinPath.
228-259: LGTM!Directory creation with proper validation and error handling.
331-378: LGTM!Snapshot write logic is well-structured with proper error recovery. The two-phase write strategy and its limitations are clearly documented.
Also applies to: 386-416
445-452: LGTM!Proper content extraction for change detection.
481-488: LGTM!Clean immutable implementation using destructuring.
302-302: No compatibility issue withstructuredCloneat this version.The minimum VSCode version is 1.95.0, which uses Electron 30+ (Chromium 124+).
structuredCloneis fully supported in VSCode 1.67+ (Electron 18+, available since 2022). The code is safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 498-511: The disableSnapshots() method is updating the wrong
setting key ('snapshots.enabled'); change the key passed to
configurationService.updateSetting to the full registered name
'deepnote.snapshots.enabled' so the ConfigurationTarget.Workspace update affects
the actual setting defined in package.json; leave the rest of the try/catch and
messages intact and ensure the call in disableSnapshots() uses the updated key
string.
- Around line 513-526: The enableSnapshots function uses the wrong configuration
key ('snapshots.enabled') when calling configurationService.updateSetting;
change the key to 'deepnote.snapshots.enabled' to match package.json and be
consistent with disableSnapshots; update the string passed to
configurationService.updateSetting in enableSnapshots (and verify
disableSnapshots uses the same 'deepnote.snapshots.enabled' key) so the setting
is stored under the correct namespace.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/common/constants.ts
🔇 Additional comments (7)
package.json (2)
96-105: LGTM: Command definitions follow established patterns.The new snapshot commands are properly structured and consistently placed alongside related explorer commands.
1677-1682: LGTM: Configuration is properly scoped and documented.The resource-scoped boolean with opt-in default (false) is the correct choice for a workspace-level feature toggle.
src/platform/common/constants.ts (1)
226-227: LGTM: Command constants properly defined.Values match package.json command IDs exactly. Placement is logical.
package.nls.json (1)
253-254: LGTM: Localization entries are clear and properly referenced.Titles match package.json references and follow established naming patterns.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
4-4: LGTM: Necessary imports added.ConfigurationTarget and IConfigurationService properly imported for snapshot configuration management.
Also applies to: 20-20
150-153: LGTM: Dependency injection properly implemented.IConfigurationService correctly injected and positioned before existing dependency.
218-221: LGTM: Command registration follows established patterns.Both commands properly registered with disposable tracking and bound to private handler methods.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @package.json:
- Around line 1653-1658: The configuration description for the
deepnote.snapshots.enabled property is not localized; replace the hard-coded
"description" value with a localization key following the project's pattern
(e.g., "%deepnote.configuration.snapshotsEnabled.description%") in the
package.json entry for deepnote.snapshots.enabled, and add the corresponding key
and translated string to the repository's localization file (e.g.,
package.nls.json) so it matches other properties that use
"%deepnote.configuration...%" keys.
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 549-577: The two methods disableSnapshots and enableSnapshots are
nearly identical; extract a shared helper (e.g., setSnapshotsEnabled(enabled:
boolean)) that takes a boolean, calls
this.configurationService.updateSetting('snapshots.enabled', enabled, undefined,
ConfigurationTarget.Workspace), shows the corresponding info message (use
conditional text based on enabled) and logs on error with the same error
handling; then replace disableSnapshots and enableSnapshots to call
setSnapshotsEnabled(false) and setSnapshotsEnabled(true) respectively to remove
duplication.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/common/constants.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/platform/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
error(30-37)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (5)
src/platform/common/constants.ts (1)
226-227: LGTM!Constants align with package.json commands and follow existing conventions.
package.nls.json (1)
253-254: LGTM!Localization keys follow established patterns.
package.json (1)
96-105: LGTM!Commands properly structured with localized titles.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
153-156: LGTM!DI wiring follows Inversify patterns correctly.
224-227: LGTM!Command registration consistent with existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 227-235: The snapshot branch uses
snapshotFileService.stripOutputsFromBlocks which only removes outputs but does
not break circular references, causing yaml.dump to fail; update the logic so
that when snapshotFileService.isSnapshotsEnabled() is true you first strip
outputs (via stripOutputsFromBlocks) and then protect against cycles by passing
the result through cloneWithoutCircularRefs (or modify stripOutputsFromBlocks to
perform an equivalent deep clone-without-cycles), ensuring notebook.blocks is
assigned a cycle-safe structure before serialization (references:
stripOutputsFromBlocks, cloneWithoutCircularRefs,
snapshotFileService.isSnapshotsEnabled, notebook.blocks).
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 43-45: The generateTimestamp function currently uses
toISOString().replace(/:/g, '-').slice(0, 19) which intentionally truncates
milliseconds for a filename-safe timestamp; add a concise comment above the
generateTimestamp function stating that milliseconds are intentionally removed
for filename safety/consistency and that the slice(0, 19) is deliberate, or if
milliseconds are required change the slice to include the fractional seconds and
update the replacement logic accordingly—refer to generateTimestamp to locate
where to add the comment or adjust the slice.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileServiceTypes.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/notebooks/deepnote/snapshotFileService.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/deepnote/snapshotFileService.node.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
ISnapshotFileService(13-13)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteOutput(15-15)DeepnoteFile(14-14)DeepnoteBlock(14-14)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (16)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
58-62: Constructor injection looks correct.Optional DI for
ISnapshotFileServiceproperly wired with@optional()decorator.
119-144: Snapshot merge flow is sound.Graceful fallback to baseline cells on error. The
cellsreassignment pattern is clear.One minor note: consider extracting the merge block into a private method for readability if this grows.
src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-57: Interface well-defined.Clean separation of types. Symbol token pattern correct for inversify DI.
src/notebooks/deepnote/snapshotFileService.unit.test.ts (4)
11-20: Test setup is clean.Using
serviceAnyfor private method access is pragmatic. Consider adding a comment explaining this pattern for future maintainers.
361-391: Config mock pattern could cause hanging tests.Per coding guidelines: "When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests."The
WorkspaceConfigurationmock is returned synchronously fromgetConfiguration, so this specific case is fine. But worth verifying if any mocks are returned from promises elsewhere.
571-597: Helper function reduces duplication.
createProjectData()is reused well across test cases.
685-709: Good resilience test.Verifying timestamped path returned even when latest write fails confirms the recovery behavior documented in the implementation.
src/notebooks/deepnote/snapshotFileService.node.ts (9)
20-37: Slug function is robust.Handles edge cases: empty strings, whitespace-only, special-chars-only. Throws typed error for invalid names.
56-60: Config key is unprefixed.Correctly using
'snapshots.enabled'without the'deepnote.'prefix sincegetConfiguration('deepnote')already scopes it. Based on learnings.
88-162: readSnapshot searches all workspace folders.Logic is correct: tries latest first, falls back to most recent timestamped. The
limit: 100on timestamped search is reasonable.One edge case: if two workspace folders contain snapshots for the same projectId, the first folder's latest wins. This matches expected behavior but could be documented.
168-223: parseSnapshotFile handles malformed YAML gracefully.Returns empty Map on parse errors. Good defensive coding.
303-310: Metadata mutation on clone.
structuredClonecreates a deep copy, so mutatingsnapshotData.metadatais safe. The pattern is correct.
336-383: Two-step write strategy is well-documented.Comment explains the tradeoff: not atomic, but provides recovery if latest write fails. Good.
427-445: Change detection excludes metadata.Uses
getComparableProjectContentto ignore timestamps.fastDeepEqualis efficient for deep comparison.Note: If read fails (catch block), defaults to
true(has changes). This is correct behavior.
491-498: stripOutputsFromBlocks uses object rest spread.The eslint-disable for unused
outputsis fine—destructuring to exclude is idiomatic.
504-514: extractOutputsFromBlocks is straightforward.Returns Map keyed by block ID. Skips blocks without ID or outputs.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 58-62: The constructor parameter order in DeepnoteSerializer is
non-alphabetical and could break manual instantiation: swap the positions of
snapshotFileService and snapshotService so that @inject(ISnapshotFileService)
@optional() private readonly snapshotFileService?: ISnapshotFileService appears
before @inject(ISnapshotMetadataService) @optional() private readonly
snapshotService?: ISnapshotMetadataServiceFull in the constructor signature;
preserve the existing decorators, types, access modifiers, and
IDeepnoteNotebookManager parameter, and update any manual instantiation sites or
tests that construct this class to use the new parameter order.
- Around line 119-144: Currently convertBlocksToCells is called immediately into
cells and then called again after merging snapshot outputs, causing double work;
modify DeepnoteSerializer logic to determine the source blocks first (use a
blocksToConvert variable: if snapshotFileService?.isSnapshotsEnabled() then try
to read snapshotOutputs and, if present, call
mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs) else fall
back to selectedNotebook.blocks ?? []), and only after resolving blocksToConvert
call this.converter.convertBlocksToCells(blocksToConvert) once; ensure existing
try/catch around readSnapshot is preserved and that logger.debug messages are
updated to reflect single conversion.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteSerializer.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(81-81)ISnapshotMetadataServiceFull(83-115)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
227-237: LGTM.Clean branching. Outputs stripped before clone; fallback retains default behavior.
| constructor( | ||
| @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, | ||
| @inject(ISnapshotMetadataService) @optional() private readonly snapshotService?: ISnapshotMetadataServiceFull | ||
| @inject(ISnapshotMetadataService) @optional() private readonly snapshotService?: ISnapshotMetadataServiceFull, | ||
| @inject(ISnapshotFileService) @optional() private readonly snapshotFileService?: ISnapshotFileService | ||
| ) {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Constructor param order: snapshotFileService before snapshotService per alphabetical guideline.
DI handles injection, but if manual instantiation exists, this is a breaking change. Low priority.
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteSerializer.ts around lines 58 - 62, The
constructor parameter order in DeepnoteSerializer is non-alphabetical and could
break manual instantiation: swap the positions of snapshotFileService and
snapshotService so that @inject(ISnapshotFileService) @optional() private
readonly snapshotFileService?: ISnapshotFileService appears before
@inject(ISnapshotMetadataService) @optional() private readonly snapshotService?:
ISnapshotMetadataServiceFull in the constructor signature; preserve the existing
decorators, types, access modifiers, and IDeepnoteNotebookManager parameter, and
update any manual instantiation sites or tests that construct this class to use
the new parameter order.
| let cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []); | ||
|
|
||
| logger.debug(`DeepnoteSerializer: Converted ${cells.length} cells from notebook blocks`); | ||
|
|
||
| // Merge outputs from snapshot if snapshots are enabled | ||
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | ||
| try { | ||
| const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId); | ||
|
|
||
| if (snapshotOutputs) { | ||
| logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`); | ||
| const blocksWithOutputs = this.snapshotFileService.mergeOutputsIntoBlocks( | ||
| selectedNotebook.blocks ?? [], | ||
| snapshotOutputs | ||
| ); | ||
|
|
||
| cells = this.converter.convertBlocksToCells(blocksWithOutputs); | ||
| } | ||
| } catch (error) { | ||
| logger.warn( | ||
| `DeepnoteSerializer: Failed to merge snapshot outputs for project ${projectId}, using baseline cells`, | ||
| error | ||
| ); | ||
| // Fall back to baseline cells (already set above) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inefficiency: double conversion when snapshots enabled.
Cells are converted at line 119, then reconverted at line 135 if snapshot outputs exist. Consider deferring the initial conversion:
♻️ Suggested optimization
- let cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);
-
- logger.debug(`DeepnoteSerializer: Converted ${cells.length} cells from notebook blocks`);
-
- // Merge outputs from snapshot if snapshots are enabled
- if (this.snapshotFileService?.isSnapshotsEnabled()) {
- try {
- const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId);
-
- if (snapshotOutputs) {
- logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`);
- const blocksWithOutputs = this.snapshotFileService.mergeOutputsIntoBlocks(
- selectedNotebook.blocks ?? [],
- snapshotOutputs
- );
-
- cells = this.converter.convertBlocksToCells(blocksWithOutputs);
- }
- } catch (error) {
- logger.warn(
- `DeepnoteSerializer: Failed to merge snapshot outputs for project ${projectId}, using baseline cells`,
- error
- );
- // Fall back to baseline cells (already set above)
- }
- }
+ let blocksToConvert = selectedNotebook.blocks ?? [];
+
+ // Merge outputs from snapshot if snapshots are enabled
+ if (this.snapshotFileService?.isSnapshotsEnabled()) {
+ try {
+ const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId);
+
+ if (snapshotOutputs) {
+ logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`);
+ blocksToConvert = this.snapshotFileService.mergeOutputsIntoBlocks(
+ blocksToConvert,
+ snapshotOutputs
+ );
+ }
+ } catch (error) {
+ logger.warn(
+ `DeepnoteSerializer: Failed to merge snapshot outputs for project ${projectId}, using baseline cells`,
+ error
+ );
+ }
+ }
+
+ const cells = this.converter.convertBlocksToCells(blocksToConvert);
+
+ logger.debug(`DeepnoteSerializer: Converted ${cells.length} cells from notebook blocks`);🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteSerializer.ts around lines 119 - 144,
Currently convertBlocksToCells is called immediately into cells and then called
again after merging snapshot outputs, causing double work; modify
DeepnoteSerializer logic to determine the source blocks first (use a
blocksToConvert variable: if snapshotFileService?.isSnapshotsEnabled() then try
to read snapshotOutputs and, if present, call
mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs) else fall
back to selectedNotebook.blocks ?? []), and only after resolving blocksToConvert
call this.converter.convertBlocksToCells(blocksToConvert) once; ensure existing
try/catch around readSnapshot is preserved and that logger.debug messages are
updated to reflect single conversion.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 121-142: DeepnoteSerializer is converting blocks to cells twice
when snapshots exist; defer conversion so convertBlocksToCells is only called
once after any snapshot-merge. Change the flow in the block that checks
snapshotService.isSnapshotsEnabled(): read snapshotOutputs via readSnapshot and,
if present, call mergeOutputsIntoBlocks on selectedNotebook.blocks to produce
blocksWithOutputs and then call
converter.convertBlocksToCells(blocksWithOutputs) to set cells; if no
snapshotOutputs, convert the original selectedNotebook.blocks once. Ensure any
early call to converter.convertBlocksToCells (the initial conversion) is removed
or moved so conversion happens only after the snapshot merge logic.
In @src/notebooks/deepnote/snapshotService.ts:
- Around line 517-565: The captureEnvironmentForNotebook method is using
logger.info for routine tracing; switch non-significant, verbose messages to
logger.debug (e.g., the initial call log, "Skipping capture: no state found",
"Skipping capture: already captured", "Could not find notebook document for
...", the list of available notebooks, "Found notebook, getting interpreter...",
and "environmentCapture returned undefined") while keeping important
success/failure logs (e.g., captured successfully and the error log in the
catch) as logger.info/logger.error; update references in this function
(captureEnvironmentForNotebook, environmentCaptured, environment, and the call
to environmentCapture.captureEnvironment) accordingly to reduce noise and match
getEnvironmentMetadata's logging level usage.
- Around line 150-160: SnapshotService directly instantiates
DeepnoteDataConverter via the private readonly converter = new
DeepnoteDataConverter(), which makes onExecutionComplete hard to unit-test;
change SnapshotService to accept a DeepnoteDataConverter (or an
interface/IDeepnoteDataConverter) via constructor injection (add a constructor
parameter with @inject for DeepnoteDataConverter), assign it to the existing
converter property instead of new-ing it, and update DI bindings to provide the
converter so tests can supply a mock/fake for onExecutionComplete.
- Around line 286-319: The getEnvironmentMetadata method is using logger.info
for routine/internal state checks; change those to a lower verbosity level
(e.g., logger.debug or logger.trace) to follow logging guidelines: replace the
logger.info calls that log existence of state, waiting for capture, timeout
start/finish, and the final state.environment/state.environmentCaptured checks
in getEnvironmentMetadata (references: method getEnvironmentMetadata, properties
state.capturePromise and state.environmentCaptured) with logger.debug (or
logger.trace if you prefer more verbosity) so only meaningful user-facing events
remain at info level.
- Around line 196-211: The method captureEnvironmentBeforeExecution sets
state.capturePromise = this.captureEnvironmentForNotebook(notebookUri) but
doesn't await it; modify captureEnvironmentBeforeExecution to await
state.capturePromise so callers block until the capture completes (i.e., replace
the assignment-only path with: assign the promise, await it), and after awaiting
clear state.capturePromise (set to undefined/null) to avoid leaving a stale
promise; reference captureEnvironmentBeforeExecution, state.capturePromise,
captureEnvironmentForNotebook and getOrCreateExecutionState when making the
change.
In @src/notebooks/deepnote/snapshotService.unit.test.ts:
- Around line 1066-1079: Tests only assert no-throw/undefined for environment
capture; add coverage that mocks IEnvironmentCapture.captureEnvironment to
assert the service state is updated and returned by getEnvironmentMetadata.
Update the unit tests for captureEnvironmentBeforeExecution to stub/mocks the
IEnvironmentCapture.captureEnvironment (or the concrete capture provider used by
service) to return a sample EnvironmentMetadata, call
service.captureEnvironmentBeforeExecution(notebookUri), then call
service.getEnvironmentMetadata(notebookUri) and assert the returned metadata
equals the mocked EnvironmentMetadata and any internal flags/state changed.
Ensure you reference the service instance used in the suite and the
IEnvironmentCapture.captureEnvironment method in your test doubles so the
behavior is validated end-to-end.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/snapshotService.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/platform/notebooks/cellExecutionStateService.tssrc/platform/notebooks/deepnote/types.ts
💤 Files with no reviewable changes (2)
- src/notebooks/deepnote/snapshotMetadataService.ts
- src/notebooks/deepnote/snapshotMetadataService.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
src/kernels/{kernel.ts,kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use event-driven updates via
EventEmitterfor kernel state changes and lifecycle events (willRestart, willInterrupt, onDidChangeKernels, etc.)
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/{kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use weak references or proper cleanup patterns to avoid memory leaks with notebook references in kernel tracking and lifecycle management
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/execution/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Coordinate notebook-level execution through
NotebookKernelExecutionwhich managesCellExecutionQueuefor sequential cell execution and usesCellExecutionMessageHandlerto process kernel messages
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/{kernel.ts,kernelFinder.ts,execution/*.ts,**/launcher*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Add telemetry tracking for all kernel operations (discovery, startup, execution, errors) to enable usage analytics
Files:
src/kernels/execution/cellExecutionQueue.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/notebooks/deepnote/snapshotService.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/notebooks/deepnote/snapshotService.unit.test.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/kernels/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
🧬 Code graph analysis (5)
src/platform/notebooks/cellExecutionStateService.ts (3)
src/platform/common/utils/lifecycle.ts (1)
trackDisposable(26-29)src/test/mocks/vsc/index.ts (1)
EventEmitter(65-89)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotService.ts (2)
ISnapshotService(113-113)ISnapshotService(119-143)
src/notebooks/serviceRegistry.node.ts (3)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(23-23)IEnvironmentCapture(25-30)src/notebooks/deepnote/snapshotService.ts (3)
ISnapshotService(113-113)ISnapshotService(119-143)ISnapshotMetadataService(20-20)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-149)
src/notebooks/deepnote/snapshotService.unit.test.ts (2)
src/test/vscode-mock.ts (2)
resetVSCodeMocks(57-305)mockedVSCodeNamespaces(15-15)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteFile(13-13)DeepnoteBlock(13-13)DeepnoteOutput(14-14)
src/notebooks/deepnote/snapshotService.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/deepnoteDataConverter.ts (1)
DeepnoteDataConverter(37-560)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (31)
src/platform/notebooks/cellExecutionStateService.ts (3)
41-49: LGTM!Interface is clean and follows existing patterns.
59-72: LGTM!Emitter setup mirrors existing pattern. Tracked for disposal.
74-81: LGTM!Simple notify function. Logging is appropriate.
src/kernels/execution/cellExecutionQueue.ts (1)
327-331: LGTM!Notification fires after queue drains, regardless of success/failure. Ensures listeners can react to completion.
src/notebooks/deepnote/deepnoteActivationService.ts (2)
12-12: LGTM!Import updated to new consolidated service.
33-33: LGTM!Optional injection appropriate for opt-in feature.
src/notebooks/deepnote/deepnoteSerializer.ts (3)
10-10: LGTM!Import updated to consolidated service.
225-235: LGTM!Output stripping conditional on snapshot mode. Circular ref handling applied consistently.
274-276: Index alignment is safe.
convertCellsToBlocksuses.map()on the cells array (deepnoteDataConverter.ts:167-169), guaranteeing a 1:1 correspondence. Since blocks are created directly fromdata.cellsimmediately before this loop, and the loop bounds onblocks.length, there is no risk of misalignment. The code also includes a null-check guard on line 288.src/platform/notebooks/deepnote/types.ts (1)
138-148: No action needed.
clearExecutionStatealready resets Run All mode viathis.runAllModeNotebooks.delete(notebookUri). The interface and implementation are correct as-is.src/notebooks/serviceRegistry.node.ts (2)
91-92: LGTM!Import structure cleanly separates the new service interface/implementation from the platform type re-export.
246-250: LGTM!Service registration follows established patterns. Binding to both
IExtensionSyncActivationService(for lifecycle) andISnapshotMetadataService(for backward compatibility) is correct.src/notebooks/deepnote/snapshotService.unit.test.ts (5)
1-26: LGTM!Test setup is clean. The
serviceAnypattern for testing private methods is reasonable here.resetVSCodeMocks()ensures isolation.
56-151: LGTM!Good coverage of path building: variants, slugification, and error cases.
153-393: LGTM!Immutability tests for all transform methods. Edge cases well covered.
417-588: LGTM!Fallback logic from latest to timestamped snapshots is well tested. Error handling behavior verified.
590-799: LGTM!Covers success, failure, and partial success. The test at lines 681-705 correctly verifies graceful degradation when latest write fails.
src/notebooks/deepnote/snapshotService.ts (14)
1-17: LGTM!Imports organized with third-party first, then local. Types correctly imported with
import typewhere applicable.
22-72: LGTM!Interfaces well-documented. NotebookExecutionState tracks all necessary state for snapshot metadata.
74-79: LGTM!Simple timeout error for environment capture. Correctly sets
name.
86-103: LGTM!Slugification handles edge cases and throws appropriate error for invalid names.
109-111: LGTM!Filename-safe timestamp format.
113-143: LGTM!Interface extends platform type and adds file I/O methods. Clean separation of concerns.
166-194: LGTM!Event subscriptions correctly registered with disposables for cleanup.
220-262: LGTM!Write order prioritizes timestamped (immutable) before latest. Graceful degradation on partial failure.
386-455: LGTM!Fallback from latest to timestamped snapshots is correct. Filename-based sorting works due to ISO timestamp format.
463-470: LGTM!Immutable stripping of outputs.
632-652: LGTM!State change handling is correct. Treating undefined success as true is reasonable fallback.
671-782: LGTM!Comprehensive validation with proper cleanup on all exit paths.
structuredCloneensures safe mutation.
839-889: LGTM!Proper error handling, deep cloning, and timestamp management.
934-946: No issue — contentHash is populated by design.
contentHashis intentionally initialized as an empty string during execution tracking. It's computed later indeepnoteSerializer.tsduring the serialization phase when callingaddSnapshotMetadataToBlocks(), which computes the SHA-256 hash (line 282). This two-phase approach separates execution timing concerns from content hashing.
Adds snapshot mode for Deepnote notebooks, which separates cell outputs from notebook content for cleaner version control.
When snapshots are enabled (deepnote.snapshots.enabled):
Benefits:
Usage:
Implementation
snapshotService.ts: Unified service handling both file I/O (read/write snapshots, merge/strip outputs) and execution metadata tracking (timing, environment capture). Listens to cell execution events to automatically create snapshots on completion.environmentCapture.node.ts: Captures Python environment metadata (packages, versions) before execution starts for inclusion in snapshots.deepnoteSerializer.ts: Updated to strip outputs on save when snapshots are enabled, and merge outputs from snapshots when opening notebooks.